Skip to content

fix: honor Hygon DCU SLA topk environment setting#1197

Open
starrkk wants to merge 3 commits into
ModelTC:mainfrom
starrkk:codex/hygon-sla-topk-env
Open

fix: honor Hygon DCU SLA topk environment setting#1197
starrkk wants to merge 3 commits into
ModelTC:mainfrom
starrkk:codex/hygon-sla-topk-env

Conversation

@starrkk

@starrkk starrkk commented Jun 30, 2026

Copy link
Copy Markdown

Summary

  • make the Hygon DCU sparse-attention fallback honor the same default top-k ratio used by the SLA path
  • keep the behavior controlled by SPARSE_ATTN_TOPK

Why

This fixes an inconsistency where the Hygon DCU fallback path used a different top-k default than the optimized runtime path.

Validation

  • branch rebuilt on latest ModelTC/LightX2V:main (89dfa833)
  • git diff --check passed for the PR branch

(cherry picked from commit e8ee93a79bd20dce2d084e992a8e140710f2c9b6)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the sparse attention configuration in flash_attn.py by changing the default fallback value of the SPARSE_ATTN_TOPK environment variable to 0.4 and dynamically passing topk_value instead of a hardcoded value. The review feedback recommends adding validation and error handling when parsing SPARSE_ATTN_TOPK to prevent potential runtime crashes from invalid float values or values outside the expected range.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

# Use Flash Attention 2.6.1 (ROCm version) with varlen interface
if SAPRDE_LINEAR_ATTN and int(os.getenv("USE_SLA", 0)) and q.shape[1] == k.shape[1]:
topk_value = float(os.getenv("SPARSE_ATTN_TOPK", "0.5"))
topk_value = float(os.getenv("SPARSE_ATTN_TOPK", "0.4"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Parsing the environment variable SPARSE_ATTN_TOPK directly into a float without validation can lead to runtime crashes (if the value is not a valid float) or unexpected behavior/crashes in the underlying sparse attention kernel (if the value is outside the valid range of (0.0, 1.0]). It is safer to wrap this in a try-except block and validate that the resulting value is within the expected range.

            try:
                topk_value = float(os.getenv("SPARSE_ATTN_TOPK", "0.4"))
                if not (0.0 < topk_value <= 1.0):
                    raise ValueError
            except ValueError:
                logger.warning("Invalid SPARSE_ATTN_TOPK value. Falling back to default 0.4.")
                topk_value = 0.4

@starrkk starrkk marked this pull request as ready for review June 30, 2026 05:18
@helloyongyang

Copy link
Copy Markdown
Contributor

We will check this pr.

And please pay attention to the code format:

pip install ruff pre-commit

pre-commit run --all-files

@helloyongyang

Copy link
Copy Markdown
Contributor

Do not use environment variables to control the sparsity of attention, especially since this switch only works under hygons.

You can write two separate attention classes. See:

https://github.com/ModelTC/LightX2V/blob/main/lightx2v/common/ops/attn/flash_attn.py

https://github.com/ModelTC/LightX2V/blob/main/lightx2v/common/ops/attn/sla_attn.py

@starrkk

starrkk commented Jul 1, 2026

Copy link
Copy Markdown
Author

@helloyongyang

Do not use environment variables to control the sparsity of attention, especially since this switch only works under hygons.

You can write two separate attention classes. See:

https://github.com/ModelTC/LightX2V/blob/main/lightx2v/common/ops/attn/flash_attn.py

https://github.com/ModelTC/LightX2V/blob/main/lightx2v/common/ops/attn/sla_attn.py

Thanks for the feedback. This PR mainly fixes an issue where topk does not take effect in the Hygon DCU SLA attention path. The current SLA interface uses a hardcoded topk value instead of the value from SPARSE_ATTN_TOPK.
I understand the concern about switching SLA inside flash attention via environment variables. If the preferred design is to keep flash_attn and sla_attn as separate implementations, I can update this PR and split the Hygon SLA attention into a separate class / registry entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants